-
Notifications
You must be signed in to change notification settings - Fork 516
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Address traceability context bug #2743 #2744
Address traceability context bug #2743 #2744
Conversation
Signed-off-by: pstlouis <[email protected]>
Signed-off-by: pstlouis <[email protected]>
Signed-off-by: PatStLouis <[email protected]>
Signed-off-by: PatStLouis <[email protected]>
Signed-off-by: PatStLouis <[email protected]>
Signed-off-by: PatStLouis <[email protected]>
Not pretty indeed 😅 I'm quite hesitant on this one. Could you provide some more details on how pyld fails when resolving the traceability context? Has the issue been raised with the authors of that library? |
So I've raised an issue on the pyld repo and the traceability interop repo. The For a minimal example, here's a code snippet. Its the specific combination of the traceability context, the traceability service endpoint and type. Remove any one of those 3 and the error isn't raised.
The issue is with the expanding part, which is a substep of the framing. The error returned is:
I'm really trying to find a solution here since aca-py currently won't be able to verify There's a traceability meeting tomorrow I will try to get more info but it's been over a month I noticed this issue. It bothers me that nobody else encountered it because as far as I can tell acapy is doing things the right way and not being able to expand a linked data document sounds like a significant issue. |
If I stumbled on the pyld repo and saw that it hadn't seen a commit in 4 years and had 20 open PRs and 50+ issues, I would assume the project was dead. That doesn't seem far from the truth, frankly. A quick scan for an alternative does not yield much in python. So that's too bad lol... Even if we did poke at the library and figure out what needs to be fixed, it seems like it would not help since PRs aren't getting merged. |
I did a bit of |
@dbluhm thanks for having a look into this. I also suspect the empty context has something to do with it and I have brought this up to the group. They didn't seem to share that opinion. Would you be able to tell what should the content of the The other option is to prettify this solution. We could leverage the resolver module directly, I ran into issues with injecting the profile and async calls, hence why I went with the requests module. The traceability context endpoint can be added to the constants. |
Can we call step 5.1.1 to the attention of the traceability folks in this section of the json-ld-api spec? https://w3c.github.io/json-ld-api/#algorithm |
Ah, you know what, I think I see the issue:
|
I still don't understand the function of the empty context, though. If that's truly necessary according to the traceability context authors then we could try patching the library or putting a PR out for it on the off chance it gets merged (and released....). But if it's not strictly necessary, maybe it could be dropped in a revision of the context. |
@dbluhm I will try the proposed edit for the pyld library. The maintainers are in the vc-api call I will point this out to them this afternoon and we can get a PR in if it solves the issue. Do you want to author it since you found the solution? I will also touch on this at the traceability call this afternoon and bring up the point you mentioned. |
@dbluhm so I think there was both an error in the
Now it seems step 21.1 is where the empty context fails. There are actually many terms with empty contexts. What a messy situation. I think bringing the PR in the pyld library is important since this is actually a python related bug. For the empty context I will get more information from the group. Fantastic find, we are definitely on the right track! |
Feel free to open the PR! I have no preference to being the author of the solution 🙂 |
Signed-off-by: PatStLouis <[email protected]>
@dbluhm while we have the open pr in the I added a non async version of the resolve function for the did:web: resolver module. It's a copy paste of the existing function but I used the requests package instead of Since the |
Signed-off-by: PatStLouis <[email protected]>
Signed-off-by: PatStLouis <[email protected]>
Signed-off-by: PatStLouis <[email protected]>
Quality Gate passedKudos, no new issues were introduced! 0 New issues |
@PatStLouis — can we close this with the new #2795 change? |
Sorry for the extra question — I had been looking at the open PRs in prep for the release candidate and didn’t see your comment on #2795. Doh! |
This PR is to address #2743
There is an odd bug that's been ongoing for some time where when trying to expand a did document with the
pyld
library it fails.Since this happen in a very specific condition, this PR looks to implement a workaround.
When we need to verify a json-ld VC where the issuer/verificationMethod is a
did:web:
scheme, we first manually resolve the did document with therequests
module, then extract the traceability context form the document if present, then frame the document directly....Its not pretty, but its the best solution I could come up with that has minimal impact on the existing code. If anyone has a better solution, I'm happy to change it!
@swcurran @dbluhm this is the last critical PR I would like to see in the 0.12 release